-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set cause for ClientPayloadError #8049
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8049 +/- ##
==========================================
+ Coverage 97.42% 97.48% +0.06%
==========================================
Files 107 107
Lines 32598 32600 +2
Branches 3800 3800
==========================================
+ Hits 31757 31781 +24
+ Misses 634 616 -18
+ Partials 207 203 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
We could really do with a way to also get the traceback added in StreamReader.set_exception(). Can't seem to figure it out at the moment, but ideally we'd grab the stack trace in that function and then add it the |
Maybe at worst we can just add:
Though feels like quite an awkward way to achieve it. |
That seems awkward for sure. Setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good
Backport to 3.9: 💚 backport PR created✅ Backport PR branch: Backported as #8050 🤖 @patchback |
(cherry picked from commit a379e63)
Backport to 3.10: 💚 backport PR created✅ Backport PR branch: Backported as #8051 🤖 @patchback |
(cherry picked from commit a379e63)
I think, this one may need a change note. |
Not that I've seen, I just noticed a lack of information while debugging a couple of issues. The set_exception()s are the main other ones, but as described above it's a bit awkward to figure out how to put the traceback in. Best left to a separate PR if we do it. |
@Dreamsorcerer I was thinking about all the other places as mentioned @ #4581 (comment). There's a Also, why didn't you reuse my patch @ #4581 (comment)? It's almost the same, but with better var names and the original exception repr backed into the message. |
Oh, I totally forgot about that conversation. I was just debugging something on my own yesterday and found that one missing. Doesn't look like you solved the .set_exception() yet though. |
@Dreamsorcerer I ended up pushing #8089. I haven't tried it, though. And didn't add any tests. Plz check it for obvious mistakes.
@bdraco my PR unifies that a bit. Interestingly, I've found the use of |
Should help improve some bug reports in future.